Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dogukan/etabs connector poc #406

Merged
merged 13 commits into from
Nov 28, 2024
Merged

Conversation

dogukankaratas
Copy link
Contributor

First attempt to implement DUI3 to ETABS. 🎉

etabsDUI3

Copy link

codecov bot commented Nov 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 9.30%. Comparing base (85bd017) to head (fa496c7).
Report is 1 commits behind head on dev.

Additional details and impacted files
@@          Coverage Diff          @@
##             dev    #406   +/-   ##
=====================================
  Coverage   9.30%   9.30%           
=====================================
  Files        224     224           
  Lines       4234    4234           
  Branches     471     471           
=====================================
  Hits         394     394           
  Misses      3824    3824           
  Partials      16      16           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bjoernsteinhagen
Copy link
Contributor

@dogukankaratas thinking about folder directories: ETABS/22/Speckle.Connectors.ETABS22 ? For future versions (same approach as Rhino)

image

@bjoernsteinhagen
Copy link
Contributor

Working on my side too, nice @dogukankaratas! 🚀

image

Copy link
Contributor

@bjoernsteinhagen bjoernsteinhagen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to be sorted in this PR but something for us to keep in mind:

  • Manual loading of the .dll file to get the plugin in ETABS? No way to get it recognised automatically? Maybe we contact ETABS support.

@oguzhankoral
Copy link
Member

@dogukankaratas @bjoernsteinhagen please do not merge this PR in before having approval from @JR-Morgan or @adamhathcock

@dogukankaratas
Copy link
Contributor Author

dogukankaratas commented Nov 25, 2024

CI Build Failure:
CI checks are failing due to a local DLL dependency for CSIAPIv1. We're currently using a local DLL because there is no official NuGet package for it.

Background:

Required Action:

  • Need to publish a new NuGet package for the net8 compatible version
  • @JR-Morgan is already aware of this situation

Note: Once published, we can replace the local DLL reference with the new NuGet package.

- Migrated the proof-of-concept to a Shared project
- Some renaming headache
- Use of Speckle.CSI.API NGet package (thanks Jedd)
- Basic selection info works
- Ready for CNX-828 and CNX-835
- Renaming of the solution structure(s) outdated in the Local.sln
@bjoernsteinhagen
Copy link
Contributor

bjoernsteinhagen commented Nov 28, 2024

Changes

  • From ETABS\Speckle.Connector.ETABS22.csproj to CSi\ETABS22\Speckle.Connectors.ETABS22.csproj and CSi\Shared\Speckle.Connectors.CSiShared.shproj
  • NuGet Speckle.CSI.API instead of dll reference

Check Failing

Running ./build.ps1 test in my terminal works no problem, PR is another thing.

I suspect Speckle.Connectors.sln and Local.sln are out of sync following all the folder movement and renaming from ETABS/Speckle.Connector.ETABSS22.csproj to CSI/ETABS22/Speckle.Connectors.ETABS22.csproj, @JR-Morgan able to give me a hand with this tomorrow?

But on a good note, we're interacting with the model and selecting things.

image

cc: @dogukankaratas, @oguzhankoral

@@ -0,0 +1,63 @@
// NOTE: Plugin entry point must match the assembly name, otherwise hits you with a "Not found" error when loading plugin
// TODO: Move ETABS implementation to csproj as part of CNX-835 and/or CNX-828
namespace Speckle.Connectors.ETABS22;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong namespace?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Temp solution, that's why I marked as TODO. ETABS expects cPlugin and Form1 namespace match assembly name. When it was under CSiShared, it gave the "Not Found" error. Will make it more elegant in next series of Ticket(s)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very annoying


// NOTE: Plugin entry point must match the assembly name, otherwise hits you with a "Not found" error when loading plugin
// TODO: Move ETABS implementation to csproj as part of CNX-835 and/or CNX-828
namespace Speckle.Connectors.ETABS22;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong namespace?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as cPlugin.cs :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also rename this form to "SpeckleForm" or something more descriptive

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This caused us headache 😆 It needs to (for some reason) be named Form1.cs @dogukankaratas and I can't explain yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is gross

Connectors/CSi/Speckle.Connectors.CSiShared/Form1.cs Outdated Show resolved Hide resolved
public CSiSharedDocumentModelStore(IJsonSerializer jsonSerializerSettings)
: base(jsonSerializerSettings) { }

protected override void HostAppSaveState(string modelCardState) => throw new NotImplementedException();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this not throw yet?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, not yet

@bjoernsteinhagen bjoernsteinhagen merged commit a99083f into dev Nov 28, 2024
5 checks passed
@bjoernsteinhagen bjoernsteinhagen deleted the dogukan/etabs-connector-poc branch November 28, 2024 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants